Skip to content

chore: refactor knip CI to fail on issues and simplify comment format#2003

Merged
kodiakhq[bot] merged 8 commits intomainfrom
brandon/knip-error
Apr 1, 2026
Merged

chore: refactor knip CI to fail on issues and simplify comment format#2003
kodiakhq[bot] merged 8 commits intomainfrom
brandon/knip-error

Conversation

@brandon-pereira
Copy link
Copy Markdown
Member

@brandon-pereira brandon-pereira commented Mar 27, 2026 •
edited
Loading

Summary

Simplify the knip GitHub Action and expand where it runs:

  • Remove main-branch comparison — knip now reports all issues found on the current branch rather than diffing against main. This makes the output more straightforward and actionable.
  • Fail on errors — the job calls core.setFailed() when knip finds issues, so the check shows as failed on the PR.
  • Run on push to main — added push trigger so knip also runs when commits land on main.
  • Support fork PRs — on fork PRs, the GITHUB_TOKEN can't write to the base repo, so the action skips PR commenting and logs results to the Actions console instead.

How to test locally or on Vercel

  1. Open a PR from a non-fork branch — knip should comment on the PR with results
  2. Push to main — knip should run and log results to the Actions console without attempting to comment
  3. (Fork PR) — knip should run, log results to the console, and skip commenting

References

  • Related PRs:

@brandon-pereira
chore: refactor knip CI to fail on issues and simplify comment format
5ae1afd 
- Remove main branch comparison (no more table/diff)
- Only comment when issues are found; delete stale comments when clean
- Refactored comment to a flat per-category breakdown of issues
- Job fails via core.setFailed() when knip finds issues
- Removed excluded categories (enumMembers, duplicates) from workflow
- Added test fixtures (unused export + unused file) to verify workflow
@brandon-pereira brandon-pereira added the ai-generated AI-generated content; review carefully before merging. label Mar 27, 2026
@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Mar 27, 2026 •
edited
Loading

⚠️ No Changeset found

Latest commit: cc44e1b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link
Copy Markdown

vercel bot commented Mar 27, 2026 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview, Comment Apr 1, 2026 5:07pm

Request Review

@brandon-pereira
fix: parse unused files from issues array in knip JSON output
f25e0af 
The knip JSON reporter nests unused files under issues[].files[] (not
a top-level files array). This fixes the parsing so unused files are
correctly reported in the PR comment.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 27, 2026 •
edited
Loading

E2E Test Results

✅ All tests passed • 123 passed • 3 skipped • 1030s

Status Count
✅ Passed 123
❌ Failed 0
⚠️ Flaky 3
⏭️ Skipped 3

Tests ran across 4 shards in parallel.

View full report →

@brandon-pereira brandon-pereira marked this pull request as ready for review March 27, 2026 22:57
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 27, 2026 •
edited
Loading

PR Review

  • ❌ Bug: Unused files not counted → The new parseResults function only iterates data.issues, but knip's JSON format puts unused files in a top-level data.files array (not within issue objects). The old code explicitly handled this with for (const f of data.files || []) and even had if (cat === 'files') continue in the issues loop to avoid double-counting. The new code silently misses all unused-file findings, causing underreported issue counts and potential false-green CI runs. Fix: restore the data.files top-level loop alongside the issues loop.

  • ⚠️ CI will fail on all PRs if main has pre-existing knip issues → Switching from diff-vs-main to absolute counts + core.setFailed() means any existing knip issues on main will immediately fail every PR, even clean ones. This may be intentional as a forcing function, but should be clearly communicated and any existing issues resolved before merging (the companion commit cc44e1bf suggests this was considered — confirm no issues remain on main).

  • ✅ Fork PR detection, comment cleanup, and workflow trigger changes look correct.

brandon-pereira
brandon-pereira commented Apr 1, 2026
View reviewed changes
knip.json
"project": ["src/**/*.ts"]
}
},
"ignore": ["scripts/dev-portal/**"],
Copy link
Copy Markdown
Member Author

@brandon-pereira brandon-pereira Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

existing knip issues

@brandon-pereira brandon-pereira requested review from a team and karl-power and removed request for a team April 1, 2026 17:06
@kodiakhq kodiakhq bot merged commit e856091 into main Apr 1, 2026
19 checks passed
@kodiakhq kodiakhq bot deleted the brandon/knip-error branch April 1, 2026 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@knudtty knudtty knudtty approved these changes

@karl-power karl-power Awaiting requested review from karl-power karl-power was automatically assigned from hyperdxio/hyperdx-team

Assignees

No one assigned

Labels

ai-generated AI-generated content; review carefully before merging. automerge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@brandon-pereira @knudtty